Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Colors: support multiple palettes #55

Merged
merged 4 commits into from
Jun 4, 2017
Merged

Conversation

djbe
Copy link
Member

@djbe djbe commented May 30, 2017

Refs SwiftGen/SwiftGenKit#40.

Updates the templates to support multiple color palettes (one for each file).

@AliSoftware
Copy link
Collaborator

Only drawback of using a protocol (to support multiple enums) instead of the enum directly is that it's not possible anymore to use the shorthand syntax of the enums (UIColir(named: .title)), as type inference can't work anymore here. you'll have to use the full color names.

Note that we have the same problem with the string template, it's not new. But in the 90% case where there's only one palette seems like a sad situation. Maybe we could improve that later on all templates having that problem? Not sure how though (as using a protocol for cases where we have multiple palettes is still the best option imho)

@djbe
Copy link
Member Author

djbe commented May 30, 2017

Well, users can always use the other creation methods, such as ColorName.myRed.color and L10n.Some.Thing.here.

Or we could do some advanced trickery where we check the number of tables/palettes/catalogs/... and not use protocols in those cases.

@AliSoftware
Copy link
Collaborator

Yeah the Colors.title.color are still possible but read less nice to me than a constructor. To each their own I guess.

We could indeed use protocols only if multiple palettes and direct enum if only one. Might be an improvement for later.

But in the meantime why do we still have to do Colors.title.color? I thought for strings we switched from enums to functions and constants, shouldn't we do the same here? let c: UIColor = Colors.title ?

@djbe
Copy link
Member Author

djbe commented May 30, 2017

Right now I've left it there because each case can have 2 values: a color and a rgbaValue. Should I remove the latter? It would also remove the UIColor(named:).

@AliSoftware
Copy link
Collaborator

Ah right. I'm not sure a lot of people query the rgbaValue. Maybe keep this variant with the enum as an alternate template and make the main one direct constants?

@djbe
Copy link
Member Author

djbe commented May 30, 2017

Hmmm, we can always add alternative templates if people ask for them, I'd say leave it out for now.

@djbe
Copy link
Member Author

djbe commented May 30, 2017

So, the result would be: (swift 2)

enum ColorName {
  /// <span style="display:block;width:3em;height:2em;border:1px solid black;background:#339666"></span>
  /// Alpha: 100% <br/> (0x339666ff)
  static let ArticleBody = Color(rgbaValue: 0x339666ff)
  /// <span style="display:block;width:3em;height:2em;border:1px solid black;background:#ff66cc"></span>
  /// Alpha: 100% <br/> (0xff66ccff)
  static let ArticleFootnote = Color(rgbaValue: 0xff66ccff)
  /// <span style="display:block;width:3em;height:2em;border:1px solid black;background:#33fe66"></span>
  /// Alpha: 100% <br/> (0x33fe66ff)
  static let ArticleTitle = Color(rgbaValue: 0x33fe66ff)
  /// <span style="display:block;width:3em;height:2em;border:1px solid black;background:#ffffff"></span>
  /// Alpha: 80% <br/> (0xffffffcc)
  static let Private = Color(rgbaValue: 0xffffffcc)
}

@AliSoftware
Copy link
Collaborator

AliSoftware commented May 30, 2017

Btw enum cases should only be used for things you'll likely need to switch over, that was a mistake I did when writing the first templates. We should replace each case x = … by a static let x = ColorStruct(rgbaValue: …) in all the templates we use enum with cases I guess (again, separate PR though. I should stop taking about our of scope ideas inside PRs, damn)

@AliSoftware
Copy link
Collaborator

Yup pretty much 👍

@djbe
Copy link
Member Author

djbe commented May 30, 2017

Do you want to do this in this PR, or a separate one?

@djbe djbe closed this May 30, 2017
@djbe djbe reopened this May 30, 2017
@AliSoftware
Copy link
Collaborator

Maybe this can fit in there if it's not too much changes yeah.

@djbe
Copy link
Member Author

djbe commented May 30, 2017

Sure thing, on it.

@djbe djbe force-pushed the feature/breaking/multiple-palettes branch from 7453b3d to 3e077fa Compare May 30, 2017 22:33
@AliSoftware
Copy link
Collaborator

AliSoftware commented May 31, 2017

You're gonna hate me be in fact the idea I had was more like this below. Maybe provide this variant as alternate template?

struct ColorName {
  let rgbaValue: UInt32
  var color: Color { return Color.init(named: self) }

  static let ArticleBody = ColorName(rgbaValue: 0x339666ff)
  static let ArticleFootnote = ColorName(rgbaValue:  0xff66ccff)
  
}
extension Color {
  convenience init(named color: ColorName) {
    self.init(rgbaValue: color.rgbaValue)
  }
}

Usage:

let c1: UIColor = ColorName.ArticleBody.color
let c2 = UIColor(named: .ArticleBody)

Still requires the extra .color on the struct instance to get the actual color, so like the enum solution we had before, except that now it's not an enum but a struct, because there's no point in switch-ing on the colors anyway.

@djbe djbe force-pushed the feature/breaking/multiple-palettes branch from 3e077fa to a8864ef Compare May 31, 2017 10:01
@djbe
Copy link
Member Author

djbe commented May 31, 2017

Lemme know if it's ok now, if so, I'll update the documentation samples.

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any unit test containing multiple color palettes in the Expected fixtures here? Did I miss them?

enum XCTColors {
struct XCTColors {
let rgbaValue: UInt32
var color: Color { return Color.init(named: self) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why I used .init there when I gave you my example, Color(named: self) should work I guess

@djbe djbe force-pushed the feature/breaking/multiple-palettes branch from ec0c794 to fdb3112 Compare May 31, 2017 14:08
@djbe djbe force-pushed the feature/breaking/multiple-palettes branch from e4e6ed6 to 5ec47f9 Compare May 31, 2017 14:41
@djbe djbe merged commit 90376a9 into master Jun 4, 2017
@djbe djbe deleted the feature/breaking/multiple-palettes branch June 4, 2017 17:58
@djbe djbe mentioned this pull request Aug 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants